Implement Quantity types in logical meter streams#11
Conversation
This makes sure that the time is incremented even when there are resampling errors. This requires that the starting time is one interval before, and the name of the timestamp field has also been updated to refer to its contents more specifically. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
- Extract resampling logic from `do_next` into a new `resample_metrics` function to improve modularity and readability. - Update `do_next` to accept pre-resampled metrics, simplifying its responsibilities. - Enhance error handling and logging for resampling and formula application processes. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
- Extract resampler cleanup logic into a new method - Introduce `DroppedUnusedFormulas` error kind, to signal when to call the resampler cleanup method. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Also remove an unused parameter and improve a log message. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
shsms
added
the
cmd:skip-release-notes
There was a problem hiding this comment.
Pull Request Overview
This PR introduces Quantity types (Power, Current, Voltage, ReactivePower, Energy, Frequency, Percentage) to replace raw f32 values in logical meter streams, providing type safety and proper unit handling for microgrid measurements.
- Adds a comprehensive quantity system with unit conversions and arithmetic operations
- Updates logical meter formulas to work with strongly-typed quantities instead of floats
- Refactors the actor system to handle multiple quantity types with type erasure mechanisms
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sample.rs | Makes Sample generic over quantity types |
| src/quantity/*.rs | Defines quantity types with unit conversions and operations |
| src/quantity.rs | Core quantity trait and macro system for type definitions |
| src/logical_meter/metric.rs | Associates metrics with their respective quantity types |
| src/logical_meter/logical_meter_actor.rs | Major refactor to support type-safe formula handling |
| src/logical_meter/formula/*.rs | Updates formula system to work with typed quantities |
| src/lib.rs | Adds quantity module export |
| src/error.rs | Adds new error kind for dropped formulas |
Comments suppressed due to low confidence (1)
src/logical_meter/logical_meter_actor.rs:543
- [nitpick] Changing from
tracing::warn!totracing::debug!reduces visibility of potentially important missing data issues. Consider keeping this as a warning or adding logic to determine when missing data is expected vs. problematic.
tracing::debug!(
"No data for metric {:?} in component {}",
metric,
resampler.component_id
);
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
shsms
force-pushed
the
quantities
branch
2 times, most recently
from
0be88a8 to
74b851c
Compare
|
Draft until there are tests and docs (for the new stuff). |
- Introduce a `Quantity` trait to define common operations and formatting for quantities. - Add `qty_ctor!` and `qty_format!` macros to simplify the creation and formatting of quantity types. - Implement the following quantity types: - `Current` - `Energy` - `Frequency` - `Percentage` - `Power` - `ReactivePower` - `Voltage` - Define arithmetic operations and unit conversions for each quantity type. - Establish relationships between quantities, such as `Power = Voltage * Current` and `Energy = Power * Duration`. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
There is a default generic of `f32` to maintain its old appearance. It will be removed as soon as the rest of the library is updated. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Also modify all existing metric definitions to include their respective quantity types. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
- Introduce `TypedFormulaResponseSender` enum to handle strongly-typed formula streams for `Power`, `Voltage`, `ReactivePower`, and `Current`. - Implement `TryFrom` trait for `TypedFormulaResponseSender` to enable conversion from a compile-time checked Generic `oneshot::Sender` to a run-time checked Enum. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Introduce the `Formulas` struct to encapsulate and manage active formulas grouped by quantity type. Implement methods for checking formula existence, sending subscription receivers, and starting new formulas with appropriate error handling. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This is done as follows:
- Add `QuantityType` as an associated type of the `Metric` trait.
- Make `AggregationFormula` and `CoalesceFormula` generic over
`Metric`, so that they get access to the `QuantityType`.
- Add a lot more generics to intermediate types to achieve this.
- Evaluate formulas in all 4 quantity groups in the active
`Formulas`.
- Cleanup resamplers only if they're not used in formulas in any of
the 4 groups in `Formulas`.
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Because each metric is now its own type, these checks are redundant and can be removed. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
OK, this was easier to read than I expected, still I focused more in the structure and the last commits were a bit harder to follow.
- Didn't get into the macros, I just assumed they do what I think they do.
- I didn't pay attention to the operations each quantity type supports. I assume it is based on the Python quantities and in any case it can be easily extended.
- "Add TypedFormulaResponseSender enum and TryFrom implementation": I didn't understand very well there the difference between the static part and the dynamic, runtime part, also didn't understand the send_subscription logic in the next commit.
| //! This module defines the `Percentage` quantity and its operations. | ||
|
|
||
| qty_ctor! { | ||
| #[doc = "A quantity representing a percentage (0% to 100%)."] |
There was a problem hiding this comment.
"Typically 0% to 100%"? It can also be 200% and -10% technically.
| use crate::quantity::{Quantity as _, test_utils::assert_f32_eq}; | ||
|
|
||
| #[test] | ||
| fn test_percentage() { |
florian-wagner-frequenz
left a comment
florian-wagner-frequenz
left a comment
There was a problem hiding this comment.
LGTM, some small nits & questions
| fn $fnname( | ||
| graph: &ComponentGraph<ElectricalComponent, ElectricalComponentConnection>, | ||
| _metric: M, | ||
| _metric: Self::MetricType, |
There was a problem hiding this comment.
Nit: consider removing the underscore from _metric as it is no longer unused.
| ) -> impl Future< | ||
| Output = Result< | ||
| broadcast::Receiver<Sample<<Self::MetricType as Metric>::QuantityType>>, | ||
| Error, | ||
| >, | ||
| > + Send; |
There was a problem hiding this comment.
Probably you have a good reason not to use async_trait here, but I tend to find these desugared async return types hard to read.
Can you maybe help me understand the decision against async_trait? Is it "fewer dependencies" (which is perfectly valid)?
There was a problem hiding this comment.
Yes, but eventually this wasn't enough and we needed the dyn Future produced by async_trait, because of tokio requirements. So this changes in the next PR.
| /// Used to send strongly-typed formula streams from the LogicalMeterActor back | ||
| /// to the Handle. | ||
| pub(crate) enum TypedFormulaResponseSender { | ||
| Power(oneshot::Sender<broadcast::Receiver<Sample<Power>>>), |
There was a problem hiding this comment.
Would it make sense to maybe condense this with a type binding?
But I could also see how additional indirection could be confusing. It's just that a little later we see these types repeated and my C++ scars start showing when I see >>>>.
| ) -> Result<HashSet<u64>, Error> { | ||
| let formula_key = (formula, metric); | ||
|
|
||
| let formula_engine = FormulaEngine::try_new(&formula_key.0) |
There was a problem hiding this comment.
nit: would it make sense to move the formula_key creation below here so that we can just use &formula instead? It's a bit odd to have the item as a scalar, pack it into a tuple only to then immediately read it back from there with the (somewhat opaque) tuple access syntax.
|
|
||
| match response_tx { | ||
| TypedFormulaResponseSender::Power(receiver_tx) => { | ||
| let (sender, receiver) = broadcast::channel(100); |
There was a problem hiding this comment.
Nit: Consider pulling the 100 into a named constant.
| receiver_tx: TypedFormulaResponseSender, | ||
| ) -> Result<(), Error> { | ||
| match receiver_tx { | ||
| TypedFormulaResponseSender::Power(sender) => { |
There was a problem hiding this comment.
There is quite some code duplication between the branches (which repeats for start_formulas, but I guess condensing it is impossible because the LogicaLMeterFormula type specializations differ.
Would it make sense to have these behind an Arc<dyn LogicalMeterFormulaTrait> or do you think it's not worth the additional indirection?
From what I see I wouldn't expect the methods of Formulas to be in the hot path, but I might be wrong.
This PR introduces
Quantitytypes likeCurrent,Power,Voltage, and updates the logical meter formulas to return values of these types, instead of floats.This PR also includes a number of other small improvements.